Skip to content

Unwrap pkg_config result in build script#34

Open
KarelPeeters wants to merge 1 commit into
KardinalAI:masterfrom
KarelPeeters:master
Open

Unwrap pkg_config result in build script#34
KarelPeeters wants to merge 1 commit into
KardinalAI:masterfrom
KarelPeeters:master

Conversation

@KarelPeeters
Copy link
Copy Markdown

@KarelPeeters KarelPeeters commented Oct 25, 2023

Currently the build.rs script from coin_cbc_sys looks like this (I've added type annotations to clarify):

use pkg_config::{Error, Library};

fn main() {
    let _: Result<Library, Error> = pkg_config::probe_library("cbc");
}

The failure condition of the probe is being completely ignored! This means that a failure to find the library is only reported as confusing linking errors at the end of the build process:

[...] = note: LINK : fatal error LNK1181: cannot open input file 'CbcSolver.lib'

I think the build script should be changed to unwrap() the Result instead, which would cause cargo to show this more helpful error message, much sooner during the build process:

   --- stderr
  thread 'main' panicked at coin_cbc_sys\build.rs:5:38:
  called `Result::unwrap()` on an `Err` value: `"pkg-config" "--libs" "--cflags" "cbc"` did not exit successfully: exit code: 1
  error: could not find system library 'cbc' required by the 'coin_cbc_sys' crate

  --- stderr
  Package clp was not found in the pkg-config search path.
  Perhaps you should add the directory containing `clp.pc'
  to the PKG_CONFIG_PATH environment variable
  Package 'clp', required by 'cbc', not found

@TeXitoi
Copy link
Copy Markdown
Collaborator

TeXitoi commented Apr 17, 2024

That’s a feature. If pkg-config is not installed, it can still work with some other manual configuration. With unwrap, it would fail the build.

5487a82

I don’t really know if that’s good or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KarelPeeters @TeXitoi